fix(coordinator): 🐛 validate serial number instead of masking missing data#673
Merged
Conversation
… data - Remove the unused catch_luxtronik_errors decorator (defined and tested, but never wired into any write path) instead of leaving dead code that looked like it protected something. - Make async_shutdown explicitly disconnect the client via a public Luxtronik.disconnect() (executor job), instead of relying on `del` + GC/__del__ timing to eventually close the socket. - Split write and identification failures out of UpdateFailed into dedicated LuxtronikWriteError / LuxtronikSerialNumberError exceptions, since UpdateFailed is meant to signal a failed update_method, not an arbitrary write or a missing serial number. - get_sensor() now tolerates coordinator.data being unpopulated instead of raising AttributeError; serial_number raises LuxtronikSerialNumberError instead of silently falling back to "" - since it feeds unique_id, an empty fallback risks colliding device/entry identifiers. - connect_and_get_coordinator() now checks last_update_success after the config-flow path's async_refresh() (which swallows failures, unlike async_config_entry_first_refresh()), converting a silent failed refresh into the existing, well-handled LuxtronikConnectionError. - Propagate LuxtronikSerialNumberError through config_flow with a dedicated "cannot_identify" abort reason/form error (all 5 locales), instead of surfacing as a generic "unknown" error. - Add regression test coverage for the above, plus the previously untested `except AbortFlow: raise` guard in async_step_dhcp and the reload_on_update=False safeguard that keeps DHCP rediscovery from re-triggering HA's "has an update listener" reload deprecation warning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🐛 Summary
Follow-up hardening from a review of
coordinator.py, focused on failing loudly instead of masking problems:catch_luxtronik_errorsdecorator — it was defined and had tests, but was never applied anywhere, so it protected nothing.async_shutdownnow explicitly closes the connection via a new publicLuxtronik.disconnect()(run in an executor job), instead ofdel-ing the client and hoping GC/__del__closes the socket before shutdown completes.UpdateFailedinto dedicatedLuxtronikWriteError/LuxtronikSerialNumberErrorexceptions —UpdateFailedis meant for a failedupdate_method, not an arbitrary write or a missing serial number.get_sensor()now toleratescoordinator.databeing unpopulated (returnsNone) instead of raisingAttributeError.serial_numbernow raisesLuxtronikSerialNumberErrorinstead of silently falling back to""when the date parameter is missing — since it feedsunique_id, an empty fallback risks colliding device/entry identifiers between two different heat pumps.connect_and_get_coordinator()now checkslast_update_successafter the config-flow path'sasync_refresh()(which swallows failures, unlikeasync_config_entry_first_refresh()), converting a silently-failed initial refresh into the existing, well-handledLuxtronikConnectionError— so problems surface at the connection step rather than later as a confusing serial-number error.config_flownow catchesLuxtronikSerialNumberErroreverywherecoordinator.unique_idis read (select_devices,manual_entry,dhcp,reconfigure) and shows a dedicatedcannot_identifyabort/error message (added to all 5 locales) instead of a generic "unknown" error.✅ Test plan
pytest— 753 passedpytest --cov=custom_components.luxtronik2— 100% coverage (including the previously-untestedexcept AbortFlow: raiseguard inasync_step_dhcp)ruff check/ruff format --check— cleanbasedpyright— 0 errorsreload_on_update=Falsesafeguard (which prevents HA's "has an update listener and should use it for scheduling a reload" deprecation warning) is untouched, and added a regression assertion for it🤖 Generated with Claude Code